add the sub-offset operator - #2483
Conversation
|
@evantypanski Any thoughts? |
|
Hey sorry I haven't gotten to this, I'll get to it either today or tomorrow. I've had some pressing stuff I wanted to get out of the way first :) thanks for your patience! While waiting, to what extent did you use AI for this PR? Zeek has an AI policy, we don't have it in Spicy yet, but probably should. |
evantypanski
left a comment
evantypanski
left a comment
There was a problem hiding this comment.
The core looks right to me, thanks! I had a couple comments on tests because when playing with it, I found it's actually slightly tricky for views. Some tests would help. Feel free to add more edge cases, too.
And my question before will still stand: did you use AI for this PR, and to what extent? Just so we can keep track
| constexpr Tag ToUIntBinary = 931; | ||
| constexpr Tag Unequal = 932; | ||
| constexpr Tag UpperCase = 933; | ||
| constexpr Tag SubOffset = 934; |
There was a problem hiding this comment.
It's kind of annoying, but could you move this next to SubOffsets? So this'd be 921, then you bump the number for all the subsequent tags.
That's what we've done eg here: bf92771
| constexpr Tag SubOffsets = 3315; | ||
| constexpr Tag UnequalBytes = 3316; | ||
| constexpr Tag UnequalView = 3317; | ||
| constexpr Tag SubOffset = 3318; |
There was a problem hiding this comment.
same as before, can this be 3316 then bump the other 2?
| assert y2.sub(y2.find(b"def")[1], end(y2)) == b"de"; | ||
| assert y.sub(3) == b"012"; | ||
| assert y.sub(0) == b""; | ||
| assert y.sub(|y|) == y; |
There was a problem hiding this comment.
We currently lack a test from hilti/spicy for an offset that's too large, mind adding one here? It should clamp. Untested suggestion:
assert y.sub(|y| + 1337) == y;
| assert v.sub(3) == b"123"; | ||
| assert v.sub(0) == b""; | ||
| assert v.sub(|v|) == b"123456789"; | ||
| assert all.sub(5) == b"01234"; |
There was a problem hiding this comment.
similar to my comment above on bytes, but views are different, it'd be nice to enshrine that in a test. v.sub(1337) will grow with a cap at 1337 if the underlying stream grows. For example, run this, it'll pass:
module Test {
global stream s = stream(b"abc");
global view<stream> my_view = s;
global view<stream> subbed = my_view.sub(1337);
assert subbed == b"abc";
s += b"def";
assert subbed == b"abcdef";
}
with:
hiltic -j test.hlt
We should test something along those lines. That's tricky behavior, and different from bytes.
EDIT: Oh, and what if the view sub was within range, but then appending puts it out? Like this:
module Test {
import hilti;
global stream s = stream(b"abc");
global view<stream> my_view = s;
global view<stream> subbed = my_view.sub(5);
assert subbed == b"abc";
s += b"a lot more than 5";
assert subbed == b"abca ";
}
Testing this behavior would be nice, since I don't actually know if we have runtime tests for this case. The more the merrier
|
2 participants
Fixes #2335
Added
Tested
subon bytes and stream views withboundary cases (
0, mid, full length), a const-operand case, and aSpicy-syntax assert (
tests/hilti/types/bytes/ops.hlt,tests/hilti/types/stream/view.hlt,tests/spicy/types/bytes/operators.spicy)